Skip to content

compiletest: Improve diagnostics for line annotation mismatches #140622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petrochenkov
Copy link
Contributor

When some line annotations are missing or misplaced, compiletest reports an error, but the error is not very convenient.
This PR attempts to improve the user experience.

  • The "expected ... not found" messages are no longer duplicated.
  • The proc_res.status and proc_res.cmdline message is no longer put in the middle of other messages describing the annotation mismatches, it's now put into the end.
  • Compiletest now makes suggestions if there are fuzzy matches between expected and actually reported errors (e.g. the annotation is put on a wrong line).
  • Missing diagnostic kinds are no longer produce an error eagerly, but instead treated as always mismatching kinds, so they can produce suggestions telling the right kind.

I'll post screenshots in the thread below, but the behavior shown on the screenshots can be reproduced locally using the new test tests/ui/compiletest-self-test/line-annotation-mismatches.rs.

r? @jieyouxu

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@petrochenkov
Copy link
Contributor Author

Before: that's the only shown error when some annotation kind is missing.

before-bad

@petrochenkov
Copy link
Contributor Author

Before: without missing annotation kinds (some stuff at the top is cropped, but the picture is clear).
before

@petrochenkov
Copy link
Contributor Author

After: with top and bottom messages merged, and suggestions added.
after

@jieyouxu
Copy link
Member

jieyouxu commented May 3, 2025

Nice! I'll play around with this tmrw on Monday.

@bors
Copy link
Collaborator

bors commented May 5, 2025

☔ The latest upstream changes (presumably #140599) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 5, 2025

@jieyouxu
By the way, do you think the line1:column1: line2:column2: part is useful in error messages?

I does two things

  • it's displayed in annotation mismatch messages, but it mostly duplicates the file:line part which is also displayed.
    • the column part could be slightly useful in the past when we didn't have UI tests with captured .stderr, but probably not now
  • it can actually be matched in line annotations, e.g. you can write //~ ERROR 28:16: cannot find value, but it seems to be an antipattern

Also, this part is not always added, it's only added to primary diagnostics, but not to labels or suggestions, for example.

I think maybe just drop it to reduce noise and duplication.
(The column can be moved to file:line:column instead of being fully dropped, if necessary.)

@petrochenkov
Copy link
Contributor Author

Another possible improvement is to display relative paths to test files instead of absolute paths, also to avoid noise, duplication and rightward shift.

The main requirement for the paths is to be "clickable", so you can quickly go from a file:line string in shell to that file and line in editor.

Relative paths here would work for me (vscode + rust is the only directory open in the workspace), but I'm not sure what other setups people can use.

@jieyouxu
Copy link
Member

jieyouxu commented May 10, 2025

Sorry for the wait, I played around with this locally, and I think it's an improvement. I have some feedback:

  1. There's a starting message

    Blessing the stderr of in "/home/joe/repos/rust/tests/ui/compiletest-self-test/line-annotation-mismatches.stderr"

    where I think the actual source file path is missing. That might be pre-existing though.
    We could also maybe highlight the paths in yellow/magenta, but yeah.

  2. I think we should only report the source file path once, /home/joe/repos/rust/tests/ui/compiletest-self-test/line-annotation-mismatches.rs, otherwise repeating that seems very noisy if we do it for every mismatch, and the rightwards drift is kinda annoying.

  3. I don't find the line1:column1: line2:column2: useful in error messages themselves, especially not in error annotations like //~ ERROR 23:22 (unless someone wants to match on the exact column but...).

  4. The column numbers might be useful if you want to jump in exactly, e.g. with neovim and whatever you can nvim path.rs:LINE:COL, or if there's > 1 diagnostics emitted on the same line? But for me, the line number is usually what matters. Agreed on stderr snapshots. I guess it might be useful if you don't check in a stderr snapshot (//@ dont-check-compiler-stderr)? I would be fine dropping it, and see if people actually have a specific use case where they want the exact column number too.

  5. I'm fine with showing only a relative path, although if we go with (2), I don't think it would matter as much.

Misc design feedback

For the actual messages, I might even do sth like:

expected but with a different kind: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:22:
          message: WARN: cannot find value `unresolved2` in this scope
    expected kind: ERROR
    reported kind: WARN

reported with a different message: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:25:
    expected message: NOTE: not found in this scope
    reported message: 

If possible, we should report both the one we got and the one we expect (kind, message, line number).

Questions

  • Also, how does this handle normalizations? E.g. //@ normalize-stderr (haven't played around with this much, I'll do that tmrw).

@jieyouxu
Copy link
Member

Let me know what u think.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 10, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants